-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix spacing of checkboxes with multi-line labels #293
Conversation
In trying to test this locally, it looks like it might not have totally fixed it, but it could be getting overridden by the temporary fix the Vets.gov team put in for this issue. @rtluu can you provide a link to the fix from the Vets.gov side so I can test that interaction? Thanks! |
hey @annekainicUSDS, the 4th chapter of this form on staging has the multiline checkbox implementation https://staging.vets.gov/education/gi-bill-school-feedback. |
Sorry, I meant the lines of code that fixed this issue that were added to Vets.gov, @rtluu. |
Ohh gotcha, @cehsu would you be able to point Anne towards the change that was made on vets-website? |
Here is the multiline label implementation on Vets.gov, please let me know if anything else would be helpful. |
@cehsu @rtluu I'm confused about the latest information provided in this ticket. It seems like we're talking about two different cases of checkbox usage. In the original ticket for this issue, you included a screenshot of a page where the second set of checkboxes had extra spacing between them. You alluded to a temporary fix added on the Vets.gov side to remove this extra spacing until a permanent fix could be applied to USFS. When I navigate to that page and inspect those checkboxes, I see a style that adds If that style is removed, I can see the extra space between the checkboxes. The style that should be adding the This first style seems like the one the Vets.gov team added to overwrite the issue. Is that correct? If so, could you please provide a link in the codebase to that style? The additional comments in this PR that include a screenshot of checkboxes with italicized text beneath them and the link to the markdown implementation of those checkboxes seems unrelated to the original reason this issue was filed, but correct me if I'm wrong on that. |
33c6fc3
to
e18b5f5
Compare
I rebased and force-pushed to ensure it won't try to put the built css file back when this is merged. |
Thanks @annekainicUSDS, they are provided here. |
@dmethvin-gov @cehsu @rtluu I've dug into this issue and PR quite a bit, and I think this particular fix is not the right solution for the problem. Here are the things I've learned:
If the Vets.gov team wants to collapse all margin between checkboxes, I believe that is a change they should keep on their side, because I don't think that's a change we want to implement for all checkbox groups in USFS. In digging into this issue, I've discovered some inconsistencies across various form examples of how checkbox groups are rendered by USFS that I think may need investigation as a separate issue. But I think this PR solving this particular problem should be closed. Let me know if you have comments or additional feedback, otherwise I will close on Monday. |
I'm fine with closing this, especially since I wasn't able to confirm the problem myself. |
Thanks for looking into this @annekainicUSDS, that's a helpful assessment and a good plan for moving forward. |
Fixes #233
I don't have a convenient way to test this, I can't get to staging. I tried playing with a codesandbox but it won't let me use a commit from a GitHub branch. @rtluu can you verify that this fixes the issue?
Types of changes
Checklist:
npm run lint
.npm run build
.npm test
.